@claude review

Claude finished @cliffhall's task in 1m 17s —— View job

PR Review: fix(filesystem): add optional depth parameter to list_directory

• Gather context and read changed files
• Analyze the diff and review the implementation
• Post review feedback

Summary

This PR adds a depth parameter to list_directory to support recursive directory listing. The motivation is valid — some LLMs generate a depth argument and get rejected by the schema. However, the implementation has several correctness bugs that would cause runtime failures.

Issues Found

1. Runtime crash — .isDirectory() called on strings (Critical)

listDirRecursive returns an array of formatted strings (e.g. "[DIR] foo"), but the calling code immediately tries to treat them as Dirent objects:

// listDirRecursive already returns pre-formatted strings like "[DIR] foo"
const entries = await listDirRecursive(validPath, 0, args.depth);

// BUG: entry is a string, not a Dirent. .isDirectory() and .name don't exist on strings.
const formatted = entries
  .map((entry) => `${entry.isDirectory() ? "[DIR]" : "[FILE]"} ${entry.name}`)
  .join("\n");

This will throw TypeError: entry.isDirectory is not a function at runtime on every call. The fix is to remove the .map() entirely and just .join("\n") on the already-formatted strings returned by listDirRecursive. Fix this →

2. Infinite recursion when depth is omitted (Critical)

The recursion guard condition is inverted:

// BUG: when maxDepth is undefined (depth not provided), this is ALWAYS true
if (entry.isDirectory() && (maxDepth === undefined || currentDepth < maxDepth)) {

When a caller omits depth, maxDepth is undefined, so maxDepth === undefined is always true, meaning the function recurses into every subdirectory indefinitely. The condition should be:

if (entry.isDirectory() && maxDepth !== undefined && currentDepth < maxDepth) {

This makes depth actually optional — when omitted, the tool lists only the immediate directory (preserving the original behavior). Fix this →

3. depth not advertised in inputSchema (Minor)

The ListDirectoryArgsSchema Zod object has depth added, but the inputSchema in registerTool does not:

// inputSchema only has path — depth is invisible to clients
inputSchema: {
  path: z.string()   // ← depth missing here
},

The handler uses ListDirectoryArgsSchema for its type but the schema advertised to MCP clients doesn't include depth. Fix this →

4. Missing TypeScript type annotations (Minor)

listDirRecursive parameters have no type annotations, which violates the repo's strict TypeScript requirement:

// TS strict mode will flag these implicit any types
async function listDirRecursive(dirPath, currentDepth, maxDepth) {

Should be:

async function listDirRecursive(dirPath: string, currentDepth: number, maxDepth: number | undefined): Promise<string[]>

5. No depth cap (Minor)

Without an upper bound, a caller can pass depth: 999 and traverse a deeply nested filesystem. Consider capping at a reasonable limit (e.g. 10) and throwing or clamping if exceeded.

6. Functional overlap with directory_tree

The existing directory_tree tool already does unbounded recursive traversal in a structured JSON format. It's worth noting this in the PR — list_directory with depth covers the "flat text" use case, so the overlap is intentional and acceptable, but reviewers should be aware.

Alternative Approach Worth Considering

The original problem is that some LLMs pass depth and get rejected. A minimal fix would be to accept-and-ignore the parameter using Zod's .passthrough(), without implementing the recursive behavior. That avoids the implementation complexity entirely if recursive listing isn't a core feature goal.

Verdict

The idea is sound, but bugs 1 and 2 are both critical and would cause this to crash or hang on every invocation. The PR needs fixes before merge.